-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: Change scroll container for sharing details #41444
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's better for the white space changes to first have commit followed by the actual change.
Hm, I don't see a point in that, can you elaborate what the benefit would be? To me having one atomic change with all required adjustemnts seems more reasonable. |
@juliushaertl Do you have understanding, what property and why fixes the problem with the scrolling? |
It's confusing for me now and it would definitely be more confusing in future if anyone looks at the commit. The fix says "Change scroll container for sharing details" then the diff shows bunch for white space changes. Unless I am missing something, I think it would be okay if the formatting changes only affected the areas that need to be reviewed. Otherwise, what if aside actual functional tests something is introduced in the unconcerned sections. That said, given, there no strict commit culture here at Nextcloud, it would be helpful then to highlight the actual changes via comment or something or best in my opinion, separate the commits (for this and similar kind of changes in the future). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested works....
Right, thanks for elaborating. This should be mostly relevant when running git blame where the |
81fdd73
to
9ffbaf6
Compare
In order to meet accessibility requirements we cannot use a sticky footer that overlaps important interactable content. Therefore this moves to a scroll container for the details which does not include the action buttons at the bottom. Larger indentation change in SharingDetailsTab was required to have a wrapping container for the scroll area. Signed-off-by: Julius Härtl <[email protected]>
9ffbaf6
to
d7ae387
Compare
/compile |
Signed-off-by: nextcloud-command <[email protected]>
I think the main lacking part was setting |
Just one detail @juliushaertl – the "Delete share" button is not supposed to be sticky, but the last button in the content area. As proposed by @JuliaKirschenheuter and approved by Michael in #41304 (comment) |
Fix #41421
Fix #41304
Replaces #41424
Best to be reviewed without whitespace changes https://github.com/nextcloud/server/pull/41444/files?w=1 as I introduce one wrapping container for the new scroll area